feat: Improve Engagement Dashboard's "Channels" tab performance #32493
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: f3ca6f0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 32 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #32493 +/- ##
===========================================
- Coverage 55.58% 55.52% -0.06%
===========================================
Files 2634 2634
Lines 57281 57255 -26
Branches 11861 11858 -3
===========================================
- Hits 31838 31790 -48
- Misses 22750 22770 +20
- Partials 2693 2695 +2
Flags with carried forward coverage won't be shown. Click here to find out more. |
Co-authored-by: Kevin Aleman <[email protected]>
|
@matheusbsilva137 since adjusting the aggregation as part of improving can you please attach the mongo explain of the query before the optimization and after? ideally with some real amount of data being processed so we can get an idea if the query is going to utilize appropriate indexes and be any better. |
There was a problem hiding this comment.
I've just attached the old and new aggregations explains to the PR description.
The new aggregation uses the t index for rooms, while the old one doesn't use indexes at all (based on what Compass told me 🤓 ). For now I'm using my local env, which doesn't have that much data, but we're planning to run this on a bigger DB for testing before this is merged.
|
Thanks for adding 🙏 this sort of data is important. Might be worth just creating a bunch of records. We should make it easier to fake a lot of data for optimizations like this. Having to wait on larger environments seems like adding in delays that we could avoid |
…nto feat/improve-engagement-dashboard-chanels
dionisio-bot
bot
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
limitas soon as possible in the aggregation so as to carry less data to the next steps;hideRoomsWithNoActivityparam to theengagement-dashboard/channels/listendpoint, which enables a faster search by not returning rooms that had no message sent in the given period. This param is being used astrueby default in our client, which will be the default behavior starting on 7.0 (when this param will be removed).Issue(s)
Steps to test or reproduce
The Engagement Dashboard's "Channels" tab should keep working just like before, but faster when there's a huge amount of rooms in the workspace. Channels should always be listed sorted by their amount of messages when no
sortparam is provided.What should be validated:
engagement-dashboard/channels/listendpoint should be sorted by their amount of new messages in the analyzed timespan (decreasingly);engagement-dashboard/channels/listendpoint should accept a new booleanhideRoomsWithNoActivityparam, which enables a faster search by removing rooms that had no messages sent in the analyzed timespan. This is specially relevant for workspaces that have a huge amount of rooms (this option is being used in the UI);engagement-dashboard/channels/listendpoint is called (so that users are aware that the new param will be deprecated);engagement-dashboard/channels/listendpoint anymore.Further comments
SUP-513
SUP-558
Aggregation explain (before): explain-aggregation-before.json
Aggregation explain (after): explain-aggregation-after.json
Aggregation explain (after and using hideRoomsWithNoActivity param): aggregation-explain-after-hide-rooms-with-no-activity.json